[NTP Next] Add updated NTP with background support #26408

zenparsing · 2024-11-06T19:57:29Z

Resolves brave/brave-browser#42298

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

brave-builds · 2024-11-06T21:08:48Z

A Storybook has been deployed to preview UI for the latest push

components/brave_new_tab/resources/lib/scoped_css.ts

components/brave_new_tab/resources/lib/app_model.ts

components/resources/brave_components_strings.grd

bridiver · 2024-11-22T23:08:43Z

components/brave_new_tab/common/BUILD.gn

+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at https://mozilla.org/MPL/2.0/.
+
+import("//mojo/public/tools/bindings/mojom.gni")


you don't need a common directory here since this is browser process only, just put everything in the root. Also there is inconsistent naming here, sometimes brave_new_tab and sometimes new_tab_page in other places. brave_new_tab_page is probably best I think

also not really sure that it makes sense to put any of this code in components unless. I would just put this code in brave/browser/ui/webui/brave_new_tab_page and put the resources in brave/browser/resources/brave_new_tab_page to match upstream

I've moved all of the code out of components as suggested - thanks! Naming is TBD.

I've updated the folder names, file names, and namespace name. I think naming is in a good place now.

I've moved the resource code back into components/brave_new_tab, as the suggested change resulted in naming conflicts with the existing NTP. Follow-up issue created here: brave/brave-browser#42563

@bridiver I came back to this again, and was able to remove the "components" folder. See this document for an analysis of the folder/naming situation and rationale for the current setup. I will be standing firm on using a C++ namespace, at least until the transition to this new WebUI is complete. When we remove the old WebUI, we can also rename folders and remove the C++ namespace, if desired.

browser/ui/webui/brave_new_tab/new_tab_page_handler.h

ui/webui/resources/BUILD.gn

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc

github-actions · 2024-12-02T12:30:55Z

Chromium major version is behind target branch (131.0.6778.85 vs 132.0.6834.15). Please rebase.

fallaciousreasoning

This is looking great @zenparsing - awesome work 😄

Sorry its taken so long to review!

fallaciousreasoning · 2024-12-08T21:36:58Z

components/brave_new_tab/new_tab_page.mojom

+interface NewTabPage {
+
+  // Called when a background-related profile preference has been updated.
+  OnBackgroundPrefsUpdated();


Hey do you think it would be useful to pass the change in here? At the moment, when you change something there are quite a few mojom round trips:

Send a message to update the pref

Receive a notification it changed from the pref handler

Send a new message to the tab handler asking about the new values

Receive the result

You're right - this can be optimized. The easiest next step would be to provide the pref name (as an enum value perhaps?) and let the front end decide what to update. I think we should leave this optimization for later, since it will take some thinking through (and it's hard to tell whether it's worth it).

Okay, happy to leave this for now. One thing to keep in mind is that it does somewhat simplify the mojom interface, as you no longer need to provide getters for any of the values (you just fire the change listener when its added). I've found the pattern makes it a bit easier to reason about where stuff comes from.

If you didn't want to add a separate listener for each different channel, you could just add all the fields into the callback

OnBackgroundPrefsUpdates(Thing1 thing1, Thing2 thing2, ..., ThingN thingN)

and you can simplify some of your initialization logic (instead, we just add ourselves as a listener and are notified of the current state + any changes that happen later).

I see that we've been moving some mojo interfaces toward a push-only API. I think that the most flexible setup is a hybrid where data is pulled and update notifications are pushed. It does require more round-trips and possibly more interface methods, however. I'll keep this in mind as we move through the project - the hybrid approach is not yet set-in-stone.

components/brave_new_tab/new_tab_page.mojom

components/brave_new_tab/resources/new_tab_page.tsx

components/brave_new_tab/resources/brave_new_tab_strings.grdp

components/brave_new_tab/resources/components/settings/background_panel.tsx

browser/ui/webui/brave_new_tab/update_observer.cc

browser/ui/webui/brave_new_tab/custom_image_chooser.cc

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc

fallaciousreasoning

Hey @zenparsing, the PR is looking great - I'm approving

Do you mind updating & resolving (with whatever you decide) these comment threads:

Thanks for all the work you've put into this! I'm going to be on PTO for a few weeks so probably best to reach out to @petemill if anything comes up 😄

browser/resources/brave_new_tab/components/app.tsx

browser/resources/brave_new_tab/components/background.tsx

browser/resources/brave_new_tab/components/background_caption.tsx

browser/resources/brave_new_tab/components/settings/background_panel.tsx

browser/resources/brave_new_tab/components/settings/settings_modal.tsx

browser/ui/webui/brave_new_tab/new_tab_page.mojom

github-actions · 2025-01-16T14:20:33Z

[puLL-Merge] - brave/brave-core@26408

Description

This PR introduces a new, updated version of the New Tab Page (NTP) for the Brave browser. It includes a new UI implementation, background image customization features, and integration with existing Brave services. The changes are feature-flagged, allowing for controlled rollout and testing.

Possible Issues

The new implementation may not cover all existing features of the current NTP, potentially leading to a temporary reduction in functionality for users who enable the new version.
The addition of new WebUI components and handlers increases the complexity of the codebase, which may require additional testing and maintenance.

Security Hotspots

CustomImageChooser: This class allows users to select files from their device. Ensure proper sanitization and validation of selected files to prevent potential security risks.
BackgroundAdapter::GetSponsoredImageBackground: This method interacts with sponsored content. Ensure that the handling of sponsored images doesn't introduce any privacy or security vulnerabilities.

Changes

Changes

.storybook/main.ts:
- Added support for stories in the browser/resources directory.
browser/about_flags.cc:
- Added a new feature flag for the updated New Tab Page.
browser/brave_browser_features.cc and browser/brave_browser_features.h:
- Defined a new feature flag kUseUpdatedNTP for the new NTP implementation.
browser/brave_content_browser_client.cc:
- Added interface binding for the new NTP handler.
browser/resources/brave_new_tab/:
- Added new directory with React components, styles, and utilities for the updated NTP.
browser/ui/webui/brave_new_tab/:
- Added new directory with C++ implementation for the WebUI backend of the new NTP.
browser/ui/webui/brave_web_ui_controller_factory.cc:
- Added conditional creation of the new NTP UI based on the feature flag.
components/resources/:
- Updated resource packing and string definitions to include the new NTP resources.

sequenceDiagram
    participant User
    participant Browser
    participant NewTabPageUI
    participant NewTabPageHandler
    participant BackgroundAdapter
    participant CustomImageChooser

    User->>Browser: Opens new tab
    Browser->>NewTabPageUI: Create UI
    NewTabPageUI->>NewTabPageHandler: Initialize
    NewTabPageHandler->>BackgroundAdapter: Get background data
    BackgroundAdapter->>NewTabPageHandler: Return background data
    NewTabPageHandler->>NewTabPageUI: Provide initial data
    NewTabPageUI->>User: Display new tab page

    User->>NewTabPageUI: Interact with settings
    NewTabPageUI->>NewTabPageHandler: Request action
    NewTabPageHandler->>BackgroundAdapter: Perform action
    alt Custom image upload
        NewTabPageHandler->>CustomImageChooser: Show file dialog
        CustomImageChooser->>User: Present file selection
        User->>CustomImageChooser: Select image(s)
        CustomImageChooser->>NewTabPageHandler: Return selected file(s)
        NewTabPageHandler->>BackgroundAdapter: Save custom background(s)
    end
    BackgroundAdapter->>NewTabPageHandler: Action result
    NewTabPageHandler->>NewTabPageUI: Update UI
    NewTabPageUI->>User: Show updated new tab page

bridiver · 2025-01-16T16:11:35Z

browser/ui/webui/brave_new_tab/custom_image_chooser.cc

+
+namespace brave_new_tab {
+
+CustomImageChooser::CustomImageChooser(content::WebContents& web_contents)


why aren't we just using the existing functionality in BraveNewTabPageHandler?

bridiver · 2025-01-16T16:17:16Z

browser/ui/webui/brave_new_tab/update_observer.cc

+
+namespace brave_new_tab {
+
+UpdateObserver::UpdateObserver(PrefService& pref_service) {


same as CustomImageChooser, why are we rewriting all of this? Per my previous comment I don't understand why we aren't just using a feature flag inside the existing ntp code. Based on the issue link it seems like it's maybe just a different design? Or I'm not even sure because there's no information in the issue other than it's supposed to allow choosing a background image which the current NTP already does

github-actions bot assigned zenparsing Nov 6, 2024

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) labels Nov 6, 2024

fallaciousreasoning reviewed Nov 6, 2024

View reviewed changes

components/brave_new_tab/resources/lib/scoped_css.ts Outdated Show resolved Hide resolved

components/brave_new_tab/resources/lib/app_model.ts Outdated Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 9 times, most recently from a4056cc to 9a7f7aa Compare November 13, 2024 16:23

zenparsing force-pushed the ksmith-ntp-update branch 7 times, most recently from 03bf3b2 to e9bef8c Compare November 21, 2024 17:42

zenparsing marked this pull request as ready for review November 21, 2024 20:05

zenparsing requested review from a team and bridiver as code owners November 21, 2024 20:05

mkarolin reviewed Nov 21, 2024

View reviewed changes

components/resources/brave_components_strings.grd Outdated Show resolved Hide resolved

zenparsing requested a review from petemill November 22, 2024 01:45

bridiver reviewed Nov 22, 2024

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page_handler.h Outdated Show resolved Hide resolved

bridiver reviewed Nov 22, 2024

View reviewed changes

ui/webui/resources/BUILD.gn Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 3 times, most recently from 070de76 to c71be60 Compare November 30, 2024 17:37

bridiver reviewed Dec 1, 2024

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc Show resolved Hide resolved

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 2, 2024

zenparsing force-pushed the ksmith-ntp-update branch from c71be60 to fc65e01 Compare December 2, 2024 12:42

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 2, 2024

fallaciousreasoning reviewed Dec 8, 2024

View reviewed changes

zenparsing force-pushed the ksmith-ntp-update branch 2 times, most recently from 364c32b to e3f3e20 Compare December 10, 2024 23:10

fallaciousreasoning reviewed Dec 11, 2024

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 3 times, most recently from 53a5151 to 4af2b2b Compare December 12, 2024 14:00

fallaciousreasoning approved these changes Dec 12, 2024

View reviewed changes

zenparsing force-pushed the ksmith-ntp-update branch 3 times, most recently from ba30e79 to a5f16d0 Compare December 20, 2024 17:42

zenparsing force-pushed the ksmith-ntp-update branch from a5f16d0 to ba6a952 Compare January 2, 2025 17:15

zenparsing force-pushed the ksmith-ntp-update branch from ba6a952 to 8c6d3d7 Compare January 14, 2025 16:27

zenparsing requested a review from a team as a code owner January 14, 2025 16:27

zenparsing force-pushed the ksmith-ntp-update branch from 8c6d3d7 to 8ec8d10 Compare January 15, 2025 16:05

github-actions bot reviewed Jan 15, 2025

View reviewed changes

petemill reviewed Jan 15, 2025

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page.mojom Outdated Show resolved Hide resolved

[NTP Next] Add updated NTP with background support

47fb748

zenparsing force-pushed the ksmith-ntp-update branch from 8ec8d10 to 47fb748 Compare January 16, 2025 14:19

bridiver reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NTP Next] Add updated NTP with background support #26408

[NTP Next] Add updated NTP with background support #26408

zenparsing commented Nov 6, 2024 •

edited

Loading

brave-builds commented Nov 6, 2024

bridiver Nov 22, 2024

bridiver Nov 22, 2024

zenparsing Nov 25, 2024

zenparsing Nov 27, 2024 •

edited

Loading

zenparsing Nov 27, 2024 •

edited

Loading

zenparsing Jan 16, 2025

github-actions bot commented Dec 2, 2024

fallaciousreasoning left a comment •

edited

Loading

fallaciousreasoning Dec 8, 2024

zenparsing Dec 10, 2024

fallaciousreasoning Dec 11, 2024

zenparsing Dec 16, 2024

fallaciousreasoning left a comment

github-actions bot commented Jan 16, 2025

Changes

bridiver Jan 16, 2025

bridiver Jan 16, 2025


		namespace brave_new_tab {

		CustomImageChooser::CustomImageChooser(content::WebContents& web_contents)


		namespace brave_new_tab {

		UpdateObserver::UpdateObserver(PrefService& pref_service) {

[NTP Next] Add updated NTP with background support #26408

Are you sure you want to change the base?

[NTP Next] Add updated NTP with background support #26408

Conversation

zenparsing commented Nov 6, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenparsing Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

zenparsing Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2024

fallaciousreasoning left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025

Description

Possible Issues

Security Hotspots

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenparsing commented Nov 6, 2024 •

edited

Loading

zenparsing Nov 27, 2024 •

edited

Loading

zenparsing Nov 27, 2024 •

edited

Loading

fallaciousreasoning left a comment •

edited

Loading